Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add aws_identitystore_groups data source #31681

Closed
wants to merge 7 commits into from
Closed

Add aws_identitystore_groups data source #31681

wants to merge 7 commits into from

Conversation

liath
Copy link
Contributor

@liath liath commented May 31, 2023

Description

Adds a data source for listing all groups in an Identity Store instance. Part of a few PRs to cover #26770.

The acceptance test is weird and I'd appreciate guidance on what to do there. The issue is that Identity Store instances are account wide so we can't guarantee there's only one group in the results or the order of the results. As far as I could find there isn't a way to say in the AttrPair checks "check these nested fields in any item of this list and compare those to these attributes on this other resource". So we're left with doing that in a custom test function.

Of course, server-side filter would likely help here but aws-sdk says that functionality is deprecated. :(

Relations

Relates #26770

References

https://docs.aws.amazon.com/singlesignon/latest/IdentityStoreAPIReference/API_ListGroups.html

Wrt to server-side filtering being deprecated in the SDK:
https://github.com/aws/aws-sdk-go-v2/blob/service/identitystore/v1.16.11/service/identitystore/api_op_ListGroups.go#L46

Output from Acceptance Testing

$ make testacc TESTS=TestAccIdentityStoreGroupsDataSource PKG=identitystore                                                                                           <aws:personal> <region:us-east-1>
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/identitystore/... -v -count 1 -parallel 20 -run='TestAccIdentityStoreGroupsDataSource'  -timeout 180m
=== RUN   TestAccIdentityStoreGroupsDataSource_basic
=== PAUSE TestAccIdentityStoreGroupsDataSource_basic
=== CONT  TestAccIdentityStoreGroupsDataSource_basic
--- PASS: TestAccIdentityStoreGroupsDataSource_basic (40.47s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/identitystore      45.534s

@github-actions
Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. generators Relates to code generators. service/identitystore Issues and PRs that pertain to the identitystore service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/L Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. and removed documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. service/identitystore Issues and PRs that pertain to the identitystore service. generators Relates to code generators. labels May 31, 2023
@liath liath mentioned this pull request May 31, 2023
Open
@justinretzolk justinretzolk added new-data-source Introduces a new data source. and removed needs-triage Waiting for first response or review from a maintainer. labels Jun 2, 2023
@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. generators Relates to code generators. service/identitystore Issues and PRs that pertain to the identitystore service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jun 12, 2023
@AndreasSko
Copy link

Hi @liath, thanks for your work here! 🙂 We are also interested in having a datasource that can list all groups in the identity center, so I appreciate that you opened this PR! ✨
I was just wondering what the state of this PR is, as it is still shown as a "draft". If understood the contributing docs right, PRs will only be reviewed when they are in the "ready" state. So maybe we could push the PR forward like this? But no worries if you are currently busy with other stuff 🙂

@liath
Copy link
Contributor Author

liath commented Aug 18, 2023

Hi @liath, thanks for your work here! 🙂 We are also interested in having a datasource that can list all groups in the identity center, so I appreciate that you opened this PR! ✨ I was just wondering what the state of this PR is, as it is still shown as a "draft". If understood the contributing docs right, PRs will only be reviewed when they are in the "ready" state. So maybe we could push the PR forward like this? But no worries if you are currently busy with other stuff 🙂

Was hoping for feedback from Hashicorp on how to proceed with the tests. I'll move it out of drafts if that's what needs to happen to get that.

@liath liath marked this pull request as ready for review August 18, 2023 22:41
@alexbacchin-asx
Copy link

Is there any chance this PR gets merged?

@liath
Copy link
Contributor Author

liath commented Jan 30, 2024

Is there any chance this PR gets merged?

Supposedly you can get Hashicorp to pay attention to PRs if you're a paying customer but we aren't lol, if you are then please send this to your account rep :)

@aristosvo
Copy link
Contributor

@bschaatsbergen Could we by any chance collaborate on this one or find a way to get this moving? I don't mind migrating this to the latest and greatest versions of framework and all, but it's now in "no man's land"

@bschaatsbergen
Copy link
Member

bschaatsbergen commented Apr 15, 2024 •
edited
Loading

@bschaatsbergen Could we by any chance collaborate on this one or find a way to get this moving? I don't mind migrating this to the latest and greatest versions of framework and all, but it's now in "no man's land"

Hey @aristosvo, cc @liath - I would be happy to help work on this PR with you two - we should begin by upgrading the code to the new plugin framework. If that's done, I can do the last couple things around tests and sign-off on the PR from my side.

How would you like to proceed collaborating?

@aristosvo
Copy link
Contributor

aristosvo commented Apr 17, 2024 •
edited
Loading

If @liath is okay with it and allows me, I'd like to push a new version with the plugin framework to this or a new PR. I have to figure out the framework a bit too as well, but that would already be a great start.

If there is a way to open up a Slack chat that would be awesome, but last time I applied for the Slack channel nothing happened 😇

@liath
Copy link
Contributor Author

liath commented Apr 18, 2024

If @liath is okay with it and allows me, I'd like to push a new version with the plugin framework to this or a new PR. I have to figure out the framework a bit too as well, but that would already be a great start.

If there is a way to open up a Slack chat that would be awesome, but last time I applied for the Slack channel nothing happened 😇

No worries here, take any of my PRs on this project lol

@aristosvo
Copy link
Contributor

Hi @liath! I've taken this PR and migrated the code to framework, would you prefer me to open up a different PR or do you prefer to give me push permissions on your branch?

Rebased version of it all can be found here: https://github.com/aristosvo/terraform-provider-aws/tree/f-aws_identitystore_groups-data-source

@liath
Copy link
Contributor Author

liath commented Apr 18, 2024

Hi @liath! I've taken this PR and migrated the code to framework, would you prefer me to open up a different PR or do you prefer to give me push permissions on your branch?

Rebased version of it all can be found here: https://github.com/aristosvo/terraform-provider-aws/tree/f-aws_identitystore_groups-data-source

I have no idea how to do that but either way lol

@aristosvo aristosvo mentioned this pull request Apr 19, 2024
Merged
@aristosvo
Copy link
Contributor

Created a new PR covering this, this one can be closed 🎉

@bschaatsbergen
Copy link
Member

Closing in favor of continuing with #36993 - thanks for picking this up @aristosvo and thanks for making sure that @liath's commits are part of it.

@liath liath mentioned this pull request Apr 23, 2024
Open
@github-actions GitHub Actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
documentation Introduces or discusses updates to documentation. generators Relates to code generators. new-data-source Introduces a new data source. service/identitystore Issues and PRs that pertain to the identitystore service. size/L Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@liath @AndreasSko @alexbacchin-asx @aristosvo @bschaatsbergen @justinretzolk